Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Copies to NEURON the voltage, i_membrane_, and mechanism data. #382

Merged
merged 6 commits into from
Aug 26, 2020

Conversation

nrnhines
Copy link
Collaborator

Framework in place.

Interacts with neuronsimulator/nrn#717

@nrnhines nrnhines marked this pull request as ready for review August 23, 2020 12:35
@pramodk
Copy link
Collaborator

pramodk commented Aug 23, 2020

@nrnhines : can we update test to check mechanism data returned by coreneuron against the neuron mechanism data? test is here: https://github.com/BlueBrain/CoreNeuron/blob/master/tests/jenkins/neuron_direct.py. Comparing few obvious variables would be sufficient I assume.

@nrnhines
Copy link
Collaborator Author

The NEURON side contains a full test. Is that not sufficient? This is only used in the corenrn_embedded case.

@pramodk
Copy link
Collaborator

pramodk commented Aug 23, 2020

The NEURON side contains a full test. Is that not sufficient? This is only used in the corenrn_embedded case.

when coreneuron side is changed, the PR on coreneuron repo doesnt trigger NEURON tests. So updating neuron_direct.py on CoreNEURON side will make sure we are not introducing bug in the master branch of CoreNEURON.

Copy link
Collaborator

@pramodk pramodk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nrnhines : overall looks good for me. Just left few remarks but already approving it.

}
}

for (NrnThreadMembList* tml = nt.tml; tml; tml = tml->next) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a remark : we can start using auto more : for (auto& tml = nt.tml;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this really work here without changing NrnThreadMembList into std::vector or giving it some kind of explicit iterator operator?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto just deduce the type from assignment. If nt.tml becomes container like std::vector then we have to use for(auto& tml : nt.tml).

double** mdata = nullptr;
NrnThread& nt = nrn_threads[tid];

n = (*nrn2core_type_return_)(0, tid, data, mdata); // 0 means time
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neuron doesn't have mechanism type 0 and that is the reason we are using first parameter0 to nrn2core_type_return_?

I wonder if we should change this to some different value (e.g. < 0) in case above assumption change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change this to some different value
Good idea Should add to the enum (on both NEURON and CoreNEURON side).

@alexsavulescu alexsavulescu merged commit 07bf5f1 into master Aug 26, 2020
@alexsavulescu alexsavulescu deleted the data-return branch August 26, 2020 11:56
pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Nov 2, 2022
…rain/CoreNeuron#382)

* Copies to NEURON the voltage, i_membrane_, and mechanism data.

Framework in place.

* voltage and i_membrane_ transfer implemented.

* Data return for mechanisms.

* Completes the data-return implementation. Needs further testing.

* Data return for NrnThread._t. Inverse permute was reversed!

* For direct mode, only call modl_reg once.

CoreNEURON Repo SHA: BlueBrain/CoreNeuron@07bf5f1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants